-
Notifications
You must be signed in to change notification settings - Fork 104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] [fix]: Address UMIP-179 feedback #602
base: master
Are you sure you want to change the base?
Conversation
pxrl
commented
Apr 22, 2024
- Specify proposer action on invalid FilledV3Relay repaymentChainId.
- Clarify proposer action on un-executable RelayerRefundLeaf.
- Add link to OZ audit for Across V3.
- Specify proposer action on invalid FilledV3Relay repaymentChainId. - Clarify proposer action on un-executable RelayerRefundLeaf. - Add link to OZ audit for Across V3.
If a validated `FilledV3Relay` event specifies an invalid `repaymentChainId`, the proposer shall issue repayment on the fill destination chain. | ||
Reasons for the `repaymentChainId` to be considered invalid are: | ||
- `repaymentChainId` is not supported by Across. | ||
- `inputToken` is not supported as a repayment token on `repaymentChainId`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think forcing repayment on the destination chain is the only safe approach here, since if the relayer fills via a contract on zkSync and the proposer proposes repayment on some other chain then the relayer will never receive their repayment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good point. I think that's right.
- The LP fee is typically referenced as a multiplier of the `V3FundsDeposited` `inputAmount`, named `realizedLpFeePct` elsewhere in this document. | ||
- The LP fee as a multiplier of `inputAmount` (typically named `realizedLpFeePct` elsewhere in this document) shall be: | ||
- Promoted to 18 decimals, where 1e18 represents 100%, and | ||
- Truncated (zeroed) between 0 and 1e10, providing 8 decimals of effective precision. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would appreciate feedback on the wording here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it's easier to understand if you show the math?
The LP fee is truncated via the following formula: output = input / 1e10 * 1e10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe:
- Available precision to 8 decimals. Truncated via [output = input / 1e10 * 1e10]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! A few minor comments. Only issue I see is that one part is ambiguous.
- The LP fee is typically referenced as a multiplier of the `V3FundsDeposited` `inputAmount`, named `realizedLpFeePct` elsewhere in this document. | ||
- The LP fee as a multiplier of `inputAmount` (typically named `realizedLpFeePct` elsewhere in this document) shall be: | ||
- Promoted to 18 decimals, where 1e18 represents 100%, and | ||
- Truncated (zeroed) between 0 and 1e10, providing 8 decimals of effective precision. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it's easier to understand if you show the math?
The LP fee is truncated via the following formula: output = input / 1e10 * 1e10
|
||
### Computing Bundle LP Fees | ||
The bundle LP fee for a target block range on a SpokePool and token pair shall be determined by summing the applicable LP fees for each of the following validated events: | ||
- `FilledV3Relay` | ||
|
||
### Computing Relayer Repayments | ||
For a validated `FilledV3Relay` event, the relayer repayment amount shall be computed as follows: | ||
- `(inputAmount * (1 - realizedLpFeePct)) / 1e18`, where `realizedLpFeePct` is computed over the set of HubPool `l1Token`, `originChainId` and `repaymentChainId` at the HubPool block number corresponding to the relevant `V3FundsDeposited` `quoteTimestamp`. | ||
- `(inputAmount * (1e18 - realizedLpFeePct)) / 1e18`, where `realizedLpFeePct` is computed over the set of HubPool `l1Token`, `originChainId` and `repaymentChainId` at the HubPool block number corresponding to the relevant `V3FundsDeposited` `quoteTimestamp`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
If a validated `FilledV3Relay` event specifies an invalid `repaymentChainId`, the proposer shall issue repayment on the fill destination chain. | ||
Reasons for the `repaymentChainId` to be considered invalid are: | ||
- `repaymentChainId` is not supported by Across. | ||
- `inputToken` is not supported as a repayment token on `repaymentChainId`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good point. I think that's right.
@@ -380,6 +385,12 @@ The set of relayer refund leaves shall be ordered according to: | |||
|
|||
The Relayer Refund Leaf `leafId` field shall be numbered according to the ordering established above, starting at 0. | |||
|
|||
If a Relayer Refund Leaf would be unable to be executed due to an ERC20 reversion when transferring the repayment token to the recipient on the destination chain, the proposer may exclude the relayer repayment from the Relayer Refund Root. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things:
- Long term, I think this would be better handled in the contracts (having a data structure to store unexecuted payments that can be claimed anytime).
- I think the way this is written, it's ambiguous. I think we should say to be excluded from the bundle, it needs to be unexecutable at the bundle end block of the chain where the repayment will take place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two possible copy changes
- The LP fee is typically referenced as a multiplier of the `V3FundsDeposited` `inputAmount`, named `realizedLpFeePct` elsewhere in this document. | ||
- The LP fee as a multiplier of `inputAmount` (typically named `realizedLpFeePct` elsewhere in this document) shall be: | ||
- Promoted to 18 decimals, where 1e18 represents 100%, and | ||
- Truncated (zeroed) between 0 and 1e10, providing 8 decimals of effective precision. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe:
- Available precision to 8 decimals. Truncated via [output = input / 1e10 * 1e10]
|
||
Note: | ||
- If a particular relayer refund is known to be unexecutable, it can be removed from the bundle by the proposer if a sufficient public justification is made before the proposal. This is intended to deal with unlikely situations, such as ag centralized token issuer blacklisting an address that is due a refund. If this leaf were to remain unaltered, this blacklisted address could block other addresses from recieving refunds. | ||
Across v3 has been [audited by OpenZeppelin](https://blog.openzeppelin.com/across-v3-incremental-audit). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Across v3 has been [audited by OpenZeppelin](https://blog.openzeppelin.com/across-v3-incremental-audit). | |
Across v3 has been audited by OpenZeppelin, shared [here](https://blog.openzeppelin.com/across-v3-incremental-audit) and [here](https://blog.openzeppelin.com/across-v3-and-oval-incremental-audit). |